Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only copy collection when really necessary #48

Merged

Conversation

felixfontein
Copy link
Contributor

@felixfontein felixfontein commented Nov 9, 2022

Subset of #46 that is not really related to change detection.

Avoid copying the part of the repository that contains the collection if the correct filesystem structure is already present. This ensures that the git repository data is kept intact.

@felixfontein
Copy link
Contributor Author

CC @webknjaz

@webknjaz webknjaz self-requested a review November 14, 2022 14:13
COLLECTION_META_FILE = 'galaxy.yml'
with open(COLLECTION_META_FILE) as galaxy_yml:
with open(os.path.join(directory, COLLECTION_META_FILE)) as galaxy_yml:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is running on a modern enough Python, maybe it's time to just use pathlib?
This all could be as simple as

from pathlib import Path
...
galaxy_yml_path = Path(directory / COLLECTION_META_FILE)
galaxy_yml_text = galaxy_yml_path.read_text()
collection_meta = yaml.loads(galaxy_yml_text)

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to merge this now but in the future, it would be better to refactor it.

@webknjaz webknjaz merged commit bff93b7 into ansible-community:main Nov 14, 2022
@felixfontein felixfontein deleted the only-copy-when-necessary branch November 14, 2022 17:23
@felixfontein
Copy link
Contributor Author

@webknjaz thanks for reviewing and merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants